[CustomDescriptors] Ensure a non-nullable type for be_on_cast_desc_eq on null - #8975
[CustomDescriptors] Ensure a non-nullable type for be_on_cast_desc_eq on null#8975kripken wants to merge 10 commits into
Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Should update the title to say br_on_cast_desc_eq rather than ref.cast_desc_eq.
| (type $0 (sub (descriptor $2) (struct))) | ||
| ;; CHECK: (type $1 (sub $0 (descriptor $3) (struct))) | ||
| (type $1 (sub $0 (descriptor $3) (struct))) | ||
| ;; CHECK: (type $2 (sub (describes $0) (struct))) | ||
| (type $2 (sub (describes $0) (struct))) | ||
| ;; CHECK: (type $3 (sub $2 (describes $1) (struct (field i32)))) | ||
| (type $3 (sub $2 (describes $1) (struct (field i32)))) |
There was a problem hiding this comment.
Can we use any in the test instead of defining any types?
There was a problem hiding this comment.
We need to br_on_cast_desc_eq on a defined descriptor type? Otherwise we get Fatal: test/lit/passes/local-subtyping-desc.wast:65:5: error: cast target must have descriptor
There was a problem hiding this comment.
Oh right, of course. Can we get away with just defining a single describee/descriptor pair, then?
There was a problem hiding this comment.
Good point, the subtyping was not needed. Simplified.
| (local.set $2 | ||
| (br_on_cast_desc_eq $block (ref null $1) (ref null $1) | ||
| (ref.null $1) | ||
| (local.get $1) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The test says we can refine local $2 and therefore the br_on_cast_desc_eq must be refinalized to the correct type. But isn't that backward and self-justifying? Without the change to finalization, we should not be trying to refine local $2 in the first place. If this refinement is a new optimization, that's one thing, but I don't see how the change can be a correct bug fix.
There was a problem hiding this comment.
This is a indeed confusing so I added a detailed comment now about exactly how the types get refined here during the 2 iterations of the pass. Hopefully that makes things clearer?
This is not an optimization, it fixes an assertion. Though I suppose it is also better and might optimize some things? Btw, this fixes a whole set of fuzzer bugs that were very annoying to track down - both I and AI gave up on most of them. This was the first testcase that was practical to investigate (and luckily it solves the others too).
kripken
changed the title
| (local $struct (ref $desc)) | ||
| (local $desc (ref $struct)) |
There was a problem hiding this comment.
The names and types are reversed!
There was a problem hiding this comment.
After the last simplification, there is just one variable (and I fixed it)
| ;; | ||
| ;; * The first time, the types of its inputs are ref=nullref, desc=(ref $desc). | ||
| ;; That refinalizes into (ref none) for the BrOn. | ||
| ;; * The first time, the types of its inputs are ref=nullref, desc=(ref none). |
There was a problem hiding this comment.
| ;; * The first time, the types of its inputs are ref=nullref, desc=(ref none). | |
| ;; * The second time, the types of its inputs are ref=nullref, desc=(ref none). |
There was a problem hiding this comment.
Can we simplify the test to only require a single iteration?
There was a problem hiding this comment.
I found a way: rather than 2 iterations, I added a block with a type that refinalize refines. This removes a local too, making us a lot shorter.
2 participants
If the desc is null, the cast is not even executed, making it unreachable.
We typed it as nullable if the ref was (in the code path
type = ref->type;),but emiting an uninhabitable type is more precise, and also avoids a fuzz
bug (see new test) where refinalize actually un-refined a type: it turned it
from non-nullable to nullable.